Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: Move handling of context arguments after handling of .depends keyword #465

Merged
merged 11 commits into from
Sep 8, 2021
Merged

BUG: Move handling of context arguments after handling of .depends keyword #465

merged 11 commits into from
Sep 8, 2021

Conversation

rrjbca
Copy link
Contributor

@rrjbca rrjbca commented May 12, 2021 •
edited
Loading

Description

Pipeline constructor now correctly infers additional arguments for Call objects after handling their extra explicit dependencies specified using the .depends keyword. Resolves #464.

Checklist

  • Follow the Contributor Guidelines
  • Write unit tests
  • Write documentation strings
  • Assign someone from your working team to review this pull request
  • Assign someone from the infrastructure team to review this pull request

@rrjbca rrjbca requested a review from a team May 13, 2021 14:00
@rrjbca rrjbca added bug Something isn't working module: pipeline labels May 13, 2021
@rrjbca rrjbca marked this pull request as ready for review May 13, 2021 14:01
@rrjbca
Copy link
Contributor Author

rrjbca commented May 13, 2021

Two further refactors I have considered. Thoughts?

  • Change the config syntax for explicit dependencies to .depends: $dependency_name i.e. a Ref instead of a string
  • Change Item.depend() to return a list of Ref objects instead of a list of strings

@ntessore
Copy link
Member

This moves the dependency structure exactly the opposite way of what I had in mind originally: I thought the Pipeline should eventually be the only entity that knows about dependencies. And the current implementation was just a half way hack to get there from how it was before.

Not that I know a better way to handle it than what is proposed here, mind. Just to add some context to why the current system is what it is.

@rrjbca
Copy link
Contributor Author

rrjbca commented May 13, 2021

Now that the PR handles dependencies all in one place (Item) rather than mixed between two (Item & Pipeline) I imagine it is possible to move the entire implementation into Pipeline if that makes more sense? I'm happy to try and make that change here.

@ntessore
Copy link
Member

I imagine it is possible to move the entire implementation into Pipeline if that makes more sense?

To me it does make more sense, because an Item does not know about the existence of other items, whereas the Pipeline knows about the existence of all the dependent objects.

I'm happy to try and make that change here.

If you want to, I think it would be a fantastic simplification, but IIRC it's potentially hard to do the dependency resolution of arguments to Calls in Pipeline. I would not make that a requirement for accepting the PR if it turns out to be challenging.

@rrjbca
Copy link
Contributor Author

rrjbca commented May 14, 2021

I have pushed a more minimal fix, @ntessore I assume you prefer this? It should also be possible to refactor such that Call.depend and Pipeline.depend don't recursively call each other which I'll think some more about. That isn't needed for this PR though and if approved I'm happy to merge as is.

@rrjbca rrjbca changed the title BUG: Move handling of .depends keyword inside Call class BUG: Move handling of context arguments after handling of .depends keyword May 14, 2021
@ntessore
Copy link
Member

Since the context for cosmology is merely a $cosmology reference, does the current solution actually add a dependency between the function call and the cosmology? That's what happened before and why the context resolution was in such an awkward place.

On the other hand, since the context mechanism was planned to add something like scope of a variable, perhaps context resolution should not introduce additional dependency by design?

@rrjbca
Copy link
Contributor Author

rrjbca commented May 14, 2021

Since the context for cosmology is merely a $cosmology reference, does the current solution actually add a dependency between the function call and the cosmology?

From looking at the code no I don't think so, but cosmology is explicitly evaluated before anything else when calling Pipeline.evaluate so technically a dependency isn't required. However this isn't the case for any other context variables which is an issue.

@rrjbca
Copy link
Contributor Author

rrjbca commented Jun 8, 2021 •
edited
Loading

@skypyproject/skypy-infrastructure could somebody please review (and approve) this fix? I will open a separate issue for the missing context dependencies identified by @ntessore above.

Currently cosmology is hardcoded as the only context variable, and it is also hardcoded to be evaluated before all other DAG jobs. Context variable dependencies in the DAG is therefore not currently an issue, but will need to be addressed when new context variables are added in the future.

@rrjbca rrjbca added this to the Version 0.4.1 milestone Aug 31, 2021
philipp128
philipp128 previously approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@philipp128 philipp128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks goof to me. Tests have passed for this.

philipp128
philipp128 approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@philipp128 philipp128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging conflict solved. Rest as before when I approved.

@rrjbca rrjbca merged commit a0ef308 into skypyproject:main Sep 8, 2021
@rrjbca rrjbca deleted the bug_464 branch September 8, 2021 14:19
rrjbca added a commit to rrjbca/skypy that referenced this pull request Sep 18, 2021
@rrjbca
BUG: Move handling of context arguments after handling of .depends ke…
1f9d0fc 
…yword (skypyproject#465)
itrharrison added a commit that referenced this pull request Dec 15, 2022
@itrharrison @rrjbca
@ntessore @Lucia-Fonseca @sutieng @philipp128 @Fox-Davidson
MAINT: merge main into module/halos (#581)
3496ac0 
* Update name of default branch to main (#434)

* update mailmap (#432)

* Write all tables to a single FITS/HDF5 file (#425)

* ADR 3: Position sampling and patches of the sky (#422)

* BUG: Raise ImportError if optional dependency speclite is not installed (#437)

* MAINT: Set NumPy latest supported version to 1.20 #440

* Update status badges (#441)

* MAINT: Update Lucia affiliation (#451)

* MAINT: add SIT's information (#450)

* DOC: Fix contributor guidelines link (#449)

Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com>
Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk>

* ENH: Logging for skypy command line script and Pipeline class (#453)

* DOC: Describe speclite filters in documentation (#457)

* ENH: Config syntax for importing objects (#463)

* DOC: List of Features (#456)

* DOC: How to construct config files (#454)

* DOC: Remove docstring examples (#429)

* MAINT: Update Zenodo entry for RPR (#468)

* DOC: Readme updates (#460)

* DOC: Expanded landing page documentation (#228)

* DOC: Inverse transform sampling accuracy warning (#472)

* MAINT: Set astropy latest supported version to 4.2 (#483)

* DOC: zenodo json members update (#481)

* DOC: Ryden04 ellipticity doc missing section (#477)

* MAINT: Update numpy and scipy latest supported versions (#488)

* BUG: Change invalid ecsv datatype from int to uint16 (#485)

* DEV: setuptools==58.0.0 (#493)

Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk>

* Add compatibility workflow badge (#487)

* DEV: Enable pip to install pre-releases in the tox dev environments (#491)

* TST: Use tmp_path fixture for temporary files in unit tests (#489)

* BUG: Move handling of context arguments after handling of .depends keyword (#465)

* BLD: Set astropy latest supported version to 4.3 and speclite minversion to 0.14 (#486)

* REV: restore setuptools to latest version on readthedocs (#494)

* DEV: pyparsing<3.0.0 (#500)

* Check new astropy file overwrite error message in logging test (#498)

* REV: restore pyparsing to latest version for doc builds (#501)

* DOC: Update citation file with JOSS paper reference (#496)

* BLD: Set astropy latest supported version to 5.0 (#504)

* BLD: Set python latest supported version to 3.10 (#505)

* BLD: Set numpy latest supported version to 1.22 (#506)

* BLD: Set python oldest supported version to 3.7 (#507)

* DOC: Fix code of conduct link (#508)

* Changed y-label in luminosity function example. (#512)

* BLD: Set scipy latest supported version to 1.8 (#510)

* ENH: Rykoff model of the magnitude uncertainty (#526)

* TST: assert photometric error is numerically close to the analytic value (#545)

* TST: Drop deprecated astropy.tests.helper.raises (#546)

* ENH: compute kcorrect remaining stellar mass (#476)

* compute kcorrect remaining stellar mass

* added test for stellar mass remain

Co-authored-by: Ian Harrison <itrharrison@gmail.com>
Co-authored-by: Lucia F. de la Bella <55983939+Lucia-Fonseca@users.noreply.github.com>

* ENH: Logistic completeness function (#521)

* BLD: Set astropy latest supported version to 5.1 (#547)

* BUG: `schechter_smf` callable input and docs (#525)

* DOC: Typo in Rykoff error (#550)

* add Fox's details (#551)

Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com>

* BLD: Set numpy latest supported version to 1.23 (#552)

* codestyle fixes

* add six requirement for colossus

* tried to fix docs builds

* update passenv

* rtd configuration

Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com>
Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk>
Co-authored-by: Lucia F. de la Bella <55983939+Lucia-Fonseca@users.noreply.github.com>
Co-authored-by: Sut-Ieng Tam <30295725+sutieng@users.noreply.github.com>
Co-authored-by: philipp128 <48715661+philipp128@users.noreply.github.com>
Co-authored-by: Fox Davidson <93545862+Fox-Davidson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipp128 philipp128 philipp128 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working module: pipeline
Projects
None yet
Milestone
Version 0.5
Development

Successfully merging this pull request may close these issues.

unexpected keyword argument '.depends'
3 participants
@rrjbca @ntessore @philipp128